Skip to content

Conversation

@franckverrot
Copy link
Contributor

Two issues are covered by this commit:

  • We make use of the value of result to define the value of the
    aggreator. Before that, finalize was used and this was not
    respecting the documentation;
  • Each call to finalize now resets the aggregator. In the future,
    there could be some kind of callback/DSL to make it configurable.

Commit by:

Two issues are covered by this commit:

  * We make use of the value of `result` to define the value of the
    aggreator. Before that, `finalize` was used and this was not
    respecting the documentation;
  * Each call to `finalize` now resets the aggregator. In the future,
    there could be some kind of callback/DSL to make it configurable.

Commit by:

  * Richard K. Michael <rmichael@edgeofthenet.org>
  * Franck Verrot <franck@verrot.fr>
tenderlove added a commit that referenced this pull request Feb 7, 2016
Align #create_aggregate with #create_aggregate_handler
@tenderlove tenderlove merged commit e833524 into sparklemotion:master Feb 7, 2016
rangeroob pushed a commit to rangeroob/sqlite3-ruby-static that referenced this pull request Apr 11, 2020
Added tests to demonstrate three issues with the way sqlite3-ruby
currently handles aggregate functions:

(1) Defining a second aggregate function and using both results in
memory violation. The problem is that `rb_iv_set(self, "@agregator",
aggregator);`   (database.c:399) just overwrites the last reference to
the first  AggregateHandler, but SQLite still holds a pointer to it and
will follow   the pointer if the first aggregate function is used in a
query.
  The most straight-forward fix is to insert the aggregator in a ruby
array and never release it -- similar to how its done for functions.

(2) Using the same aggregate function twice in a query mixes up values
from both  columns. For example:
  `SELECT MYAGGREG(a), MYAGGREG(b) FROM foo;`
  Leads to: Myaggreg.step(a1); Myaggreg.step(b1); Myaggreg.step(a2);
  Myaggreg.step(b2); ... ; Myaggreg.finalize(), Myaggreg.finalize()
  The SQLite API expects the caller to differentiate between these
chains  of invocation via `sqlite3_aggregate_context()`, but current
sqlite3-ruby  does not account for that.
  #44 has been a work around for this in the special case that the first
  aggregation is finalized before the second started (separate queries).
  sparklemotion#161 does the analog for the function Proxy.

(3) Documentation implies that library users shall explicitly set the
arity of the function. Consequently the library user is likely to
expect that this is passed down to SQLite, so he may define multiple
functions with the same name but different arity and SQLite to use the
most appropriate one  (documented sqlite feature). Unfortunately,
sqlite3-ruby does not pass the arity to SQLite, which is surprising
given that different arity-values  are accounted for in the C code. The
problem is that sqlite3-ruby does not call the AggregateHandlers
"arity" method but instead queries the arity of
  the "step" method from Ruby (sqlite3_obj_method_arity). Confusingly,
this  is not the "step" provided by the library user, but the "step" of
the  Proxy Wrapper classes introduced by Database.create_aggregate or
Database.create_aggregate_handler. Both of these are just
`step(*args)`,  so the arity reported to SQLite is always -1.

Things not addressed:
  - sparklemotion#164 impossible to call FunctionProxy.set_error (and
FunctionProxy.count).  because @driver does not exist (anymore)
  - Database.create_aggregate and the tests contain a 'text_rep' field,
that  is never passed down to SQLite. It is not advertised in the
documentation, though.

(cherry picked from commit ae89dea)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants